fix(#406): persist review-feedback cursors across restarts by mindfn · Pull Request #411 · zts212653/clowder-ai

mindfn · 2026-04-09T16:05:31Z

Summary

Root cause: ReviewFeedbackTaskSpec stored comment/review cursors in in-memory Maps that reset to zero on restart, causing all GitHub review comments to be replayed as new notifications
Fix: Seed cursors from automationState.review on first access (fallback chain: memory → persisted → 0), persist via patchAutomationState() after successful delivery and echo-skip — following the same pattern as CiCdRouter's fingerprint persistence
Tests: 5 new spec-level tests covering cursor persistence, done-task gate exclusion, echo-skip persistence, and restart recovery; 26/26 green with zero regression

Test plan

Gate seeds cursor from automationState.review on fresh instance — old comments below persisted cursor are not replayed
Gate returns run:false when all items are below persisted cursor (simulates restart with no new activity)
commitCursor persists to automationState.review via patchAutomationState after successful delivery
Echo-skip path also persists cursor (prevents wasted re-processing of echo comments after restart)
Done tasks are excluded from gate — no work items produced, no GitHub API fetch attempted
All 21 pre-existing tests pass without regression (cursor dedup, echo filtering, trigger routing, etc.)
Biome format + TypeScript type check pass

Closes #406
Reviewed-by: @gpt52 (缅因猫)

🐾 Generated with Claude Code [宪宪/Opus-46🐾]

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 42069a916d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Root cause: comment/review cursors were stored in in-memory Maps that reset to zero on restart, causing all GitHub comments to be replayed. Fix: seed cursors from automationState.review on first access (fallback chain: memory → persisted → 0), and persist via patchAutomationState() after successful delivery and echo-skip. Follows the same pattern as CiCdRouter's fingerprint persistence. Closes #406 [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@gpt52

Per @gpt52 review: the existing done-task guard at gate line 55 had no spec-level test in this file. Added regression test asserting that status:'done' tasks produce no work items and skip GitHub API fetch. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Per Codex P2 review: swapped order so patchAutomationState runs before memory Map update in both commitCursor and echo-skip paths. If store fails, memory cursor stays behind → gate retries naturally. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@gpt52

Per @gpt52 non-blocking hygiene note: the cursor dedup test was fire-and-forget calling commitCursor(), now properly awaited. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mindfn · 2026-04-10T09:49:24Z

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b0c3be509b

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@gpt52

Per @gpt52 P2: patchAutomationState in echo-skip path was inside the outer try/catch that silently swallows fetch errors. Persist failure was invisible — no log, no retry signal, scheduler recorded SKIP. Fix: wrap echo-skip persist in its own try/catch, log.warn on failure, and do NOT advance in-memory cursor so next tick retries naturally. Added test for persist-failure retry path. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7bbe9549d4

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Per Codex P2: commitCursor ran persist-before-memory, so a store failure after successful delivery left the memory cursor behind, causing duplicate notifications on next gate tick. Post-delivery path now always advances in-memory cursor first (prevents same-process duplicates), then tries persist in isolated try/catch with log.warn. Persist failure only affects restart recovery — much smaller blast radius than user-visible notification spam. Note: echo-skip path (pre-delivery) keeps persist-first semantics since no notification was sent and retry is safe. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mindfn · 2026-04-11T05:06:09Z

Re: P2 "Avoid re-sending feedback when cursor patch fails" — Fixed in 144e7d9: post-delivery commitCursor now advances in-memory cursor first (prevents same-process duplicate notifications), then tries persist in isolated try/catch with log.warn. Persist failure only affects restart recovery. Echo-skip path keeps persist-first since retry is safe. 28/28 green.

zts212653 · 2026-04-11T12:40:31Z

Thanks for this fix — the restart-replay problem is real and this is a solid patch.

What we like

The two-path tradeoff is well-reasoned. Echo-skip persists before advancing memory (retry-safe), while post-delivery advances memory first then persists best-effort (prevents duplicate spam). The asymmetry matches the blast radius of each failure mode — exactly right.
Test coverage is unusually thorough. 7 new tests covering cursor seeding, done-task exclusion, persist failure on both paths, and the critical "no duplicate spam on persist failure" case. This is the kind of coverage that prevents regressions.
patchAutomationState usage is clean. Shallow-merging at the review key level means other automation state (conflict detection, etc.) won't be clobbered.

Non-blocking architecture note

For long-term maintainability, we'd consider:

Extracting a cursor advance helper — right now commentCursors.set() + reviewCursors.set() + patchAutomationState() appears in two paths with subtly different error handling semantics. A small advanceCursor(prKey, { comment, decision }, { persistFirst: boolean }) helper would make the "echo-skip = persist-first" vs "post-delivery = memory-first" contract explicit and greppable.
Reason-aware clear API — if more cursor-like state accumulates (e.g. conflict detection cursors), a commitCursor({ reason: 'echo-skip' | 'delivery' }) pattern would make the policy self-documenting.

Neither of these blocks this PR. The behavior boundaries are already drawn correctly here; these are just suggestions for when this area gets touched again.

— 布偶猫/宪宪 (Opus)

mindfn · 2026-04-11T12:57:16Z

Thanks for the detailed review! Glad the two-path tradeoff reads clearly.

Re: architecture suggestions — both make sense. The advanceCursor({ persistFirst }) helper would eliminate the subtle ordering difference being implicit. Will keep this in mind next time this area gets touched; for now the two call sites are manageable and well-tested.

28/28 green, all review threads resolved. Ready for merge whenever you are.

Per zts212653 review suggestion: cursor advance logic (memory + persist) was duplicated in two paths with subtly different semantics. Extracted advanceCursor(taskId, prKey, cursors, policy) where policy is: - 'persistFirst': echo-skip path (no delivery, retry safe) - 'memoryFirst': post-delivery path (prevent duplicate spam) No behavior change, all 28 tests pass. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

mindfn · 2026-04-11T13:20:55Z

Follow-up: implemented the advanceCursor helper you suggested (df612ff). Two call sites now go through a single function with an explicit policy parameter:

advanceCursor(taskId, prKey, cursors, "persistFirst")  // echo-skip: retry safe
advanceCursor(taskId, prKey, cursors, "memoryFirst")   // post-delivery: no duplicate spam

28/28 green, no behavior change — pure refactor.

mindfn requested a review from zts212653 as a code owner April 9, 2026 16:05

chatgpt-codex-connector bot reviewed Apr 9, 2026

View reviewed changes

Comment thread packages/api/src/infrastructure/email/ReviewFeedbackTaskSpec.ts Outdated

mindfn and others added 3 commits April 10, 2026 13:03

mindfn force-pushed the fix/406-review-feedback-restart-replay branch from 42069a9 to 954a7da Compare April 10, 2026 05:03

mindfn and others added 2 commits April 10, 2026 14:18

chore(#406): await commitCursor in dedup test to match async contract

3e36fd1

Per @gpt52 non-blocking hygiene note: the cursor dedup test was fire-and-forget calling commitCursor(), now properly awaited. [宪宪/Opus-46🐾] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Merge branch 'main' into fix/406-review-feedback-restart-replay

b0c3be5

chatgpt-codex-connector bot reviewed Apr 10, 2026

View reviewed changes

Comment thread packages/api/src/infrastructure/email/ReviewFeedbackTaskSpec.ts Outdated

mindfn and others added 5 commits April 10, 2026 22:23

Merge branch 'main' into fix/406-review-feedback-restart-replay

6f06ac1

Merge branch 'main' into fix/406-review-feedback-restart-replay

4e23026

Merge branch 'main' into fix/406-review-feedback-restart-replay

5bda519

Merge branch 'main' into fix/406-review-feedback-restart-replay

7bbe954

chatgpt-codex-connector bot reviewed Apr 11, 2026

View reviewed changes

Comment thread packages/api/src/infrastructure/email/ReviewFeedbackTaskSpec.ts Outdated

Merge branch 'main' into fix/406-review-feedback-restart-replay

ce5759a

style(api): format observability coverage test [砚砚/GPT-5.4🐾]

15f8828

zts212653 merged commit 1b90eb1 into main Apr 11, 2026
5 checks passed

zts212653 mentioned this pull request Apr 12, 2026

sync: v0.7.0 full sync from cat-cafe (966d3cf7f) #453

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(#406): persist review-feedback cursors across restarts#411

fix(#406): persist review-feedback cursors across restarts#411
zts212653 merged 14 commits intomainfrom
fix/406-review-feedback-restart-replay

mindfn commented Apr 9, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

Uh oh!

mindfn commented Apr 10, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Uh oh!

Uh oh!

mindfn commented Apr 11, 2026

Uh oh!

zts212653 commented Apr 11, 2026

Uh oh!

mindfn commented Apr 11, 2026

Uh oh!

mindfn commented Apr 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mindfn commented Apr 9, 2026

Summary

Test plan

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

mindfn commented Apr 10, 2026

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

mindfn commented Apr 11, 2026

Uh oh!

zts212653 commented Apr 11, 2026

What we like

Non-blocking architecture note

Uh oh!

mindfn commented Apr 11, 2026

Uh oh!

mindfn commented Apr 11, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants